Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(temporal): allow customization of SSL mode for external db #361

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(temporal): allow customization of SSL mode for external db #361

wants to merge 1 commit into from
+12 −7

Conversation

CyberHippo
Copy link

What

Due to a recent chart change, it is not possible to disable SSL for external databases.
This change to the temporal chart allows to configure the SSL value for both internal and external databases.

Fixes airbytehq/airbyte#43328

How

By adding a new chart value to enable or disable SSL for database in the global section.

global:
  database:
    type: external
    ssl:
      enabled: false

This parameter is not related to the database type.

Note: The new value could be moved to the temporal section if needed. I thought that since the SSL flags were initially introduced in the global section I would add the new value there as well:

        #####
        # How the SSL flags were introduced
        #####

        {{- if eq .Values.global.database.type "external" }}
        # Assume an external database requires SSL.
          - name: POSTGRES_TLS_ENABLED
            value: "true"
          - name: POSTGRES_TLS_DISABLE_HOST_VERIFICATION
            value: "true"
          - name: SQL_TLS_ENABLED
            value: "true"
          - name: SQL_TLS_DISABLE_HOST_VERIFICATION
            value: "true"
        {{- end }}

Recommended reading order

  1. /

Can this PR be safely reverted and rolled back?

  • YES 💚 but this could break the chart as it was the case with 4731a0b
  • NO ❌

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@marcosmarxm marcosmarxm requested a review from davinchia October 1, 2024 18:40
@kev-datams
Copy link

hi @davinchia, do you have an ETA ? 🙏
this is currently a blocking topic on our side to migrate to external DB without SSL
thanks 😃

@film42
Copy link

film42 commented Oct 8, 2024

Thanks @CyberHippo for opening this. We too are broke by the previous change. Looking forward to this.

@marcosmarxm
Copy link
Member

Hello all, sorry the delay to get review in this contribution. I raised it to the platform team. After I get a return will update here.

@marcosmarxm marcosmarxm mentioned this pull request Oct 9, 2024
Open
@kev-datams
Copy link

Hi @marcosmarxm, do you have some news about this blocking issue for us ? 🙏

@AHulshoff
Copy link

Any news on when this PR is going to be merged? It is blocking for me too. I am setting up Airbyte with Flux, so the workaround to adjust the variables will not work for me, as they will be overwritten again.

@CyberHippo
Copy link
Author

Following the comment from @PurseChicken, do we still need this PR?

@kev-datams
Copy link

kev-datams commented Nov 14, 2024 •
edited
Loading

Following the comment from @PurseChicken, do we still need this PR?

Hi @CyberHippo
marking a database as internal in the YAML in order to use an external database looks confusing... I suppose the proposed solution is a temporary workaround
the ideal would be to be able to disable ssl with external as proposed in the issue 🙏

@kev-datams
Copy link

Hi @CyberHippo, do you have an ETA for the fix release ? 🙏

@CyberHippo
Copy link
Author

Hi @kev-datams , unfortunately I am not part of Airbyte platform team and don't know about their chart release process. I am also waiting for this to be released. Maybe @davinchia knows more about it?

@marcosmarxm
Copy link
Member

Apologies for the missing updates on this contribution. I'll coordinate with Davin to determine the next steps for securing a review.

@kev-datams
Copy link

Apologies for the missing updates on this contribution. I'll coordinate with Davin to determine the next steps for securing a review.

Thanks @marcosmarxm 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davinchia davinchia Awaiting requested review from davinchia

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/platform kubernetes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Platform Chart changes to Temporal Deployment causing TLS issues.
7 participants
@CyberHippo @CLAassistant @kev-datams @film42 @marcosmarxm @AHulshoff @octavia-squidington-iii